Update useParticipants to listen for any Participant's info changes #980
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: b3ec185 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
what do you think @lukasIO? |
lukasIO
approved these changes
Sep 23, 2024
|
Thanks for catching this! We missed to update those events in the event group. |
lukasIO
reviewed
Sep 23, 2024
.changeset/calm-rivers-behave.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@livekit/components-core': minor | |||
There was a problem hiding this comment.
Suggested change
| '@livekit/components-core': minor | |
| '@livekit/components-core': patch |
as it's primarily a bugfix, I think we should mark it as a patch update
There was a problem hiding this comment.
Fixed, ready for merge @lukasIO !
|
@lebaudantoine feel free to open a subsequent PRs for the other missing events, we'd definitely want them to be part of |
Extended allRemoteParticipantRoomEvents to include participant state events, aligning with @Ocupe's original intent in livekit#306: covering "all room events that trigger state changes on a participant by default." Since both name and metadata are treated as participant info in some observers, it makes sense to include these events. Name changes won’t cause frequent re-renders, but attributes might. This raises larger questions: Does the current naming accurately reflect that not all remote participant events are covered? Should participantInfoObserver also track attributes, if we consider them part of the participant's information? Feedback and edits are more than welcome
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
This is my first contribution! I’d love any advice on how to contribute to the repo. While working with the
useParticipantshook, I expected it to keep the participants list updated with their names. However, I found thatuseParticipantsonly listens to metadata changes by default and does not react to updates in names or attributes.To ensure the participants list updates correctly, I called the hook as follows:
Additionally, I noticed that the term
updateOnlyOncould be misleading, as it always includes three events. While the documentation clarifies this, it may cause confusion when reading the code.Solution
I found the previous implementation a bit convoluted. I propose that
useParticipantsshould update on all room events that trigger state changes for a participant, as originally intended by @Ocupe. Additionally, please note that a few events related to end-to-end encryption and permissions are still missing from the current setup.